Skip to content

C++: Combine results for cpp/weak-cryptographic-algorithm#6007

Merged
MathiasVP merged 1 commit into
github:mainfrom
geoffw0:weak_crypto2
Jun 4, 2021
Merged

C++: Combine results for cpp/weak-cryptographic-algorithm#6007
MathiasVP merged 1 commit into
github:mainfrom
geoffw0:weak_crypto2

Conversation

@geoffw0

@geoffw0 geoffw0 commented Jun 4, 2021

Copy link
Copy Markdown
Contributor

The issue here is that cpp/weak-cryptographic-algorithm often reports many pieces of evidence of the same issue, that a particular cryptographic algorithm is in use (see https://lgtm.com/rules/2162200683/alerts/, though that is probably still showing an older version of the query). The noise of all these extra results is particularly unpleasant when viewing them alongside other query results.

The solution in this PR is to combine all alerts in a file into a single alert at the first location, with $@ references to the other results. The results look much cleaner on LGTM.

@geoffw0 geoffw0 added the C++ label Jun 4, 2021
@geoffw0 geoffw0 requested a review from a team as a code owner June 4, 2021 09:27
MathiasVP
MathiasVP previously approved these changes Jun 4, 2021

@MathiasVP MathiasVP left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (assuming the tests pass.)

Do we need to write another change-note given that we already have one for this query from #5896?

@geoffw0 geoffw0 added the no-change-note-required This PR does not need a change note label Jun 4, 2021
@geoffw0

geoffw0 commented Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

Do we need to write another change-note given that we already have one for this query from #5896?

I don't think so. There may (hopefully!) at some point be a third PR increasing the @precision of this query to high, at which point we'll want a second change note I think.

@geoffw0

geoffw0 commented Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

LGTM! (assuming the tests pass.)

Looks like I forgot to make a PR for the internal test as well. I'll do that later today.

@geoffw0 geoffw0 added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Jun 4, 2021
@geoffw0

geoffw0 commented Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

Internal changes made. This should be ready to merge (together with the other PR) when the tests pass on that one.

@MathiasVP MathiasVP merged commit f21e949 into github:main Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants